Fix GRANDPA set ID handling - #2971
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| Grandpa::schedule_change(next_authorities, in_blocks, forced)?; | ||
| // This runtime does not use pallet-session, so mirror the bookkeeping performed by | ||
| // pallet-grandpa's session handler after it successfully schedules an authority change. | ||
| pallet_grandpa::CurrentSetId::<Runtime>::mutate(|set_id| *set_id += 1); |
There was a problem hiding this comment.
[HIGH] Delay set-ID advancement until the authority change takes effect
schedule_change accepts an arbitrary in_blocks, but this advances CurrentSetId immediately while Grandpa::grandpa_authorities() remains the old set until the scheduled block. During that window, a restarting node reads the new set ID through GrandpaApi::current_set_id() alongside the old authorities and can initialize GRANDPA with an invalid (set_id, authorities) pair, impairing finality. The session-handler pattern cited here is safe only when scheduling at the session transition; it cannot be copied unchanged to this delayed admin API. Advance the ID when the change is enacted, or constrain this path to an immediate change and reject nonzero delays. Add a restart/runtime-API regression test for the pending-change window.
🛡️ AI Review — Skeptic (security review)VERDICT: VULNERABLE BASELINE scrutiny: repository admin with matching author/committer and substantial merged history; no Gittensor association. Legitimate stacked branch targeting #2959. The warp-sync wrapper is narrowly scoped to Finney and preserves the testnet checkpoint path. However, the runtime bookkeeping change exposes an incorrect GRANDPA set ID during delayed authority changes. Findings
ConclusionThe PR is legitimate-looking, but incrementing # 🔍 AI Review — Auditor (domain review) has not yet run on this PR. |
|
🔄 AI review updated — Skeptic: VULNERABLE |
1 participant
Summary
Grandpa::CurrentSetIdafter a successful admin-scheduled authority changeRoot cause
The admin scheduling path called
Grandpa::schedule_changedirectly and therefore bypassed the set-ID bookkeeping normally performed bypallet-grandpa's session integration. This runtime does not usepallet-session, so the successful scheduling path must perform that bookkeeping explicitly.Separately, the pinned SDK's legacy initial-set-ID mechanism adds its offset inside the proof-fragment loop. Finney needs the historical offset only at the beginning of verification; applying it to every fragment over-counts later authority-set transitions.
Impact
Authority rotations scheduled through
admin-utilsnow keep on-chain GRANDPA set-ID state aligned with the scheduled transition. Finney warp-sync verification also carries the corrected set ID forward normally across subsequent transitions.Validation
CARGO_NET_OFFLINE=true cargo test -p pallet-admin-utils --lockedSKIP_WASM_BUILD=1 CARGO_NET_OFFLINE=true cargo test -p node-subtensor grandpa_warp_sync --locked -- --nocaptureSKIP_WASM_BUILD=1 CARGO_NET_OFFLINE=true cargo test -p node-subtensor-runtime --lib --lockedSKIP_WASM_BUILD=1 CARGO_NET_OFFLINE=true cargo clippy -p pallet-admin-utils -p node-subtensor-runtime -p node-subtensor --tests --no-deps --lockedcargo fmt --all -- --checkgit diff --checkDependency
This is intentionally stacked on #2959 while that PR remains open. Retarget this PR to
mainafter #2959 merges.